Skip to content

Conversation

wesleywiser
Copy link
Member

Normally, Thread::spawn takes care of setting the thread's name, if
one was provided, but since the main thread wasn't created by calling
Thread::spawn, we need to call that function in std::rt::init.

This is mainly useful for system tools like debuggers and profilers
which might show the thread name to a user. Prior to these changes, gdb
and WinDbg would show all thread names except the main thread's name to
a user. I've validated that this patch resolves the issue for both
debuggers.

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 19, 2022
@rust-highfive
Copy link
Contributor

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive
Copy link
Contributor

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 19, 2022
@ChrisDenton
Copy link
Member

This looks reasonable to me and I confirmed it worked. It would be good to add a test for it but I can see that would be awkward considering it's inherently platform-specific and I'm pretty sure the sys modules don't include a way to read the OS thread name.

@luqmana
Copy link
Contributor

luqmana commented May 20, 2022

This looks reasonable to me and I confirmed it worked. It would be good to add a test for it but I can see that would be awkward considering it's inherently platform-specific and I'm pretty sure the sys modules don't include a way to read the OS thread name.

Could probably do it via debuginfo-style tests

@wesleywiser wesleywiser force-pushed the main_thread_name branch 2 times, most recently from b55e054 to 7f4d65d Compare May 21, 2022 01:14
@wesleywiser
Copy link
Member Author

Good idea! I've added a debuginfo test.

@ChrisDenton
Copy link
Member

Since most of the libs team have a lot on their plate atm and I'm already invested here...

r? @ChrisDenton

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

Normally, `Thread::spawn` takes care of setting the thread's name, if
one was provided, but since the main thread wasn't created by calling
`Thread::spawn`, we need to call that function in `std::rt::init`.

This is mainly useful for system tools like debuggers and profilers
which might show the thread name to a user. Prior to these changes, gdb
and WinDbg would show all thread names except the main thread's name to
a user. I've validated that this patch resolves the issue for both
debuggers.
@ChrisDenton
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 2, 2022

📌 Commit 820ffc8 has been approved by ChrisDenton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 2, 2022
@bors
Copy link
Collaborator

bors commented Jun 3, 2022

⌛ Testing commit 820ffc8 with merge 32cac736087efb4919a56641d90152aac6b4ca2b...

@bors
Copy link
Collaborator

bors commented Jun 3, 2022

💔 Test failed - checks-actions

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jun 3, 2022
@bors bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 3, 2022
@rust-log-analyzer

This comment has been minimized.

@ChrisDenton
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 3, 2022

📌 Commit 7cb538b has been approved by ChrisDenton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 3, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 4, 2022
…isDenton

Call the OS function to set the main thread's name on program init

Normally, `Thread::spawn` takes care of setting the thread's name, if
one was provided, but since the main thread wasn't created by calling
`Thread::spawn`, we need to call that function in `std::rt::init`.

This is mainly useful for system tools like debuggers and profilers
which might show the thread name to a user. Prior to these changes, gdb
and WinDbg would show all thread names except the main thread's name to
a user. I've validated that this patch resolves the issue for both
debuggers.
@JohnTitor
Copy link
Member

Failed in rollup: #97727 (comment)
@bors r- rollup=iffy

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 4, 2022
@wesleywiser
Copy link
Member Author

Added // ignore-windows-gnu to the test since gdb on Windows doesn't seem to support showing the thread name.

@ChrisDenton
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 4, 2022

📌 Commit cb87ce2 has been approved by ChrisDenton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 4, 2022
@bors
Copy link
Collaborator

bors commented Jun 4, 2022

⌛ Testing commit cb87ce2 with merge 4e725ba...

@bors
Copy link
Collaborator

bors commented Jun 4, 2022

☀️ Test successful - checks-actions
Approved by: ChrisDenton
Pushing 4e725ba to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 4, 2022
@bors bors merged commit 4e725ba into rust-lang:master Jun 4, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 4, 2022
@rust-highfive
Copy link
Contributor

📣 Toolstate changed by #97191!

Tested on commit 4e725ba.
Direct link to PR: #97191

💔 miri on windows: test-pass → test-fail (cc @oli-obk @RalfJung @eddyb).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Jun 4, 2022
Tested on commit rust-lang/rust@4e725ba.
Direct link to PR: <rust-lang/rust#97191>

💔 miri on windows: test-pass → test-fail (cc @oli-obk @RalfJung @eddyb).
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4e725ba): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 🎉 relevant improvements found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
-2.8% -3.8% 4
All 😿🎉 (primary) N/A N/A 0

Cycles

Results
  • Primary benchmarks: mixed results
  • Secondary benchmarks: no relevant changes found
mean1 max count2
Regressions 😿
(primary)
2.3% 2.3% 1
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
-2.5% -2.5% 1
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) -0.1% -2.5% 2

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2

  2. number of relevant changes 2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.